-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PROD-1331 #4421
PROD-1331 #4421
Conversation
…forth when root user needs to install something.
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #5295 ↗︎
Details:
Review all test suite changes for PR #4421 ↗︎ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4421 +/- ##
=======================================
Coverage 87.10% 87.10%
=======================================
Files 329 329
Lines 20366 20366
Branches 2623 2623
=======================================
Hits 17739 17739
Misses 2163 2163
Partials 464 464 ☔ View full report in Codecov by Sentry. |
# The else statement is required due to the way commands are structured and is arbitrary. | ||
CI_ARGS = "-T" if getenv("CI") else "--user=fidesuser" | ||
CI_ARGS_EXEC = "-t" if not getenv("CI") else "--user=fidesuser" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing numerous tests to fail when running:
docker exec -e ANALYTICS_OPT_OUT -e FIDES__CLI__ANALYTICS_ID --user=root fides pytest --cov-report=xml tests/ctl/ -m 'not external'
so I updated it to use the new fidesuser
I'm a little confused about the previous code comment here: The else statement is required due to the way commands are structured and is arbitrary.
It seems like the "else" matters, is all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me either, it seems like the flags (-T or -t) control TTY settings which seems incompatible with specifying a user. The earliest occurrence of this comes from @ThomasLaPiana, maybe he can comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a super hacky workaround. Due to the way that commands were injected, it broke if I tried to use an empty string, so instead I used a "dummy" flag which ended up being --user=root
. As long as there is something valid in there it should be fine :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasLaPiana the value seemed to matter though which is why I was confused. Once I started running this container as fidesuser
and not root
, a large number of tests were failing though and changing the value here to --user=fidesuser
fixed.
Looks good to me, @pattisdr! This PR meets the security requirement:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes make sense, the only part that threw me off was the CI_ARGS
/CI_ARGS_EXEC
portion but I tagged @ThomasLaPiana to comment if he remembers. The only change that would be a nice to have is locking down the fides_uploads
to the fidesuser like you did in fidesplus.
# The else statement is required due to the way commands are structured and is arbitrary. | ||
CI_ARGS = "-T" if getenv("CI") else "--user=fidesuser" | ||
CI_ARGS_EXEC = "-t" if not getenv("CI") else "--user=fidesuser" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me either, it seems like the flags (-T or -t) control TTY settings which seems incompatible with specifying a user. The earliest occurrence of this comes from @ThomasLaPiana, maybe he can comment?
OK @galvana thanks for your review! The changes I've made as a result: I'm chowning the fides directory but leaving the fides user is as-is. |
Closes #PROD-1331
Description Of Changes
Run fides as non-root user.
Code Changes
Steps to Confirm
whoami
is fidesuser. Confirm you can run privacy requests.Pre-Merge Checklist
CHANGELOG.md